Skip to content

feat: expose attrs in typetracer#2806

Merged
agoose77 merged 4 commits into
mainfrom
agoose77/feat-expose-attrs-typetracer
Nov 8, 2023
Merged

feat: expose attrs in typetracer#2806
agoose77 merged 4 commits into
mainfrom
agoose77/feat-expose-attrs-typetracer

Conversation

@agoose77

@agoose77 agoose77 commented Nov 8, 2023

Copy link
Copy Markdown
Collaborator

This PR exposes the new attrs mapping argument in the ak.typetracer public L2 API.

@agoose77
agoose77 requested a review from jpivarski November 8, 2023 20:35

@jpivarski jpivarski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

@agoose77
agoose77 force-pushed the agoose77/feat-expose-attrs-typetracer branch from 3ad4cbb to a395351 Compare November 8, 2023 20:43
@agoose77
agoose77 enabled auto-merge (squash) November 8, 2023 20:45
@agoose77
agoose77 merged commit ec261c2 into main Nov 8, 2023
@agoose77
agoose77 deleted the agoose77/feat-expose-attrs-typetracer branch November 8, 2023 22:02
@jpivarski

Copy link
Copy Markdown
Member

Adding a HighLevelContext to touch_data and _length_0_1_if_typetracer causes to_layout to be called on the array, which falls back to to_iter, which fails because it's a typetracer array. These functions only make sense if the argument is already a typetracer array, so this effectively breaks them. It's responsible for #3077 (comment) (which is an issue about a regression, and the last version they checked is, indeed, before this PR).

The reason you added HighLevelContext here was to propagate attrs. Is there a way to propagate attrs without something that eventually goes to to_layout? I could do it the pedestrian way of just checking for the things that need to be propagated, attrs and behaviors, and propagating them. I would be in favor of that for these exceptional functions—low-level functions that need to propagate high-level state—but if there's a utility function for it already, I ought to use that.

Is there a utility function for what I'm trying to do? If not, I'll happily do it the pedestrian way.

@jpivarski

Copy link
Copy Markdown
Member

This conversation is continuing at #3077 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants